Fix: Make Bind() ViewModel parameters nullable. #2468

Noggog · 2020-07-30T02:59:55Z

What kind of change does this PR introduce?

This PR modifies the PropertyBindingMixins used mainly within ReactiveUserControl codebehind WhenActivated blocks.
It changes the viewModel members to be nullable.

What is the current behavior?
Bit of contextual overview:
The ViewModel parameter in Bind is mostly unused. The reason it exists is to simplify generic specification of the call. By passing the ViewModel in, the generics can be implied, whereas if the parameter was omitted, the user would have to type out the verbose generic args themselves for every call. Otherwise, the parameter is mostly unneeded, and theoretically could be removed.

99% of the time Bind is used, ReactiveUserControl.ViewModel member is what is passed in. The current misalignment in nullability, though, means the user has to pass in the ViewModel with the override operator:

this.Bind(this.ViewModel!, ...

What is the new behavior?

Since the ViewModel is unused, and just there for typing, it doesn't actually matter if the parameter nullable or not.

So why make it nullable?
By shifting the parameter to be nullable, it aligns with the ReactiveUserControl.ViewModel member, meaning the call is cleaner for the end user:

this.Bind(this.ViewModel, ...

This is much cleaner for the user, and is less confusing, as requiring a ! operator makes the user question if they're doing something wrong, and/or tempts them to need to check that ViewModel is not null first.. which we know is an unnecessary step.

This API shift takes the burden/questions off the user on if they're doing something wrong, and lets them use Bind more as they would expect.

What might this PR break?

I added ArgumentNullExceptions to check that the viewmodel is not null. We know in typical usage it will not be null, as WhenActivated will never be called with null ViewModels, which is where Bind is being invoked from. Still, it is good to check that it is not null internally anyway, as it definitely means something is in an error state, as this should never happen.

One reason to do so, is that the ViewModel parameter IS used in one spot: It's given to the ReactiveBinding class that is returned, which has a ViewModel member, which is not nullable. It is good to check that the VM is not null, just as it's an error state if that was to ever happen, and it means we safely fill this member with a viewmodel object that is not null, as it expects.

One note is that the current methodology of passing in this.ViewModel! is no safer, we're still potentially "lying" and setting a null view model object to ReactiveBinding's non-null member. The ArgumentNullException is actually a bit safer on that front, nipping an odd situation in the bud early.

Summary

I think this change gives the user an experience they're expecting, while retaining the safety of the system overall.

I had some remaining errors related to some missing SDKs for some of the more obscure platform targets. I think everything is okay. We'll let the build system double check, though.

ViewModel objects are passed in mainly to help enforce generic typing without the user needing to manually type in all the generics themselves. Being passed to this internal functionality as object seems unnecessary

The parameter is mainly there to simplify generic specification for the calling user. Therefore, it doesn't matter if it is null. Making the parameter nullable means it lines up cleaner with ReactiveControl's ViewModel member, which is 99% of the time what is being passed in.

glennawatson · 2020-07-30T03:04:13Z

Sorry, didn't mean to close the PR. Will try out and let you know what I find.

src/ReactiveUI/Interfaces/IPropertyBindingHook.cs

glennawatson · 2020-07-30T03:06:56Z

src/ReactiveUI.Winforms/ContentControlBindingHook.cs

@@ -17,7 +17,7 @@ namespace ReactiveUI.Winforms
    public class ContentControlBindingHook : IPropertyBindingHook
    {
        /// <inheritdoc/>
-        public bool ExecuteHook(object source, object target, Func<IObservedChange<object, object>[]> getCurrentViewModelProperties, Func<IObservedChange<object, object>[]> getCurrentViewProperties, BindingDirection direction)
+        public bool ExecuteHook(object target, Func<IObservedChange<object, object>[]> getCurrentViewModelProperties, Func<IObservedChange<object, object>[]> getCurrentViewProperties, BindingDirection direction)


Can you please remove this from this PR and make it a separate PR, doesn't seem to be related to the nullability factor.

Sure, can do. I agree it was a tangential change

This reverts commit 237817d.

Noggog · 2020-07-30T03:19:10Z

Sorry about the overeager change.

Reversed it, and the overall changes should be much cleaner now. Let me know if you want a flatten or anything.

The preceding null check /w ArgumentNullException throw made the pass to ExecuteHook clean anyway. So definitely better to leave as is.

glennawatson · 2020-07-30T03:20:16Z

It's quite common in Xamarin Forms app btw that the "ViewModel" may be null initially btw. Eg on those platforms WhenActivated isn't required always.

So the ArgumentNullException could cause issues for that platform.

Noggog · 2020-07-30T03:22:30Z

Alright, so then the solution might be to just remove ArgumentNullException, and utilize ! internally.

The only potential issue I can see is that the returned ReactiveBinding will have a TViewModel ViewModel member set to null. This is the current behavior anyway, I imagine. Still sort of a lie to say it wont be null when it potentially can be. Perhaps we'd want to flip IReactiveBinding.ViewModel to be nullable to be more accurate for those situations you mentioned in Xamarin?

Same with ExecuteHook. The source parameter claims the object wont be null, but in the Xamarin path, it may be

glennawatson · 2020-07-30T03:24:13Z

If you have the viewModel nullable without the ArgumentNullException, does that compile?

That should allow users to keep their current syntax, or at least use ? right.

Noggog · 2020-07-30T03:26:38Z

Removing the ArgumentNullException by itself will make it not compile. This is because the ReactiveBinding we're returning wants a viewmodel that is not null.
So either we need to:

check that it isn't null, and throw if it is (current setup in this PR)
Modify ReactiveBinding (and ExecuteHook) to have nullable viewmodel members
Shove it in and lie with a !

I would vote 2, as that's giving users accurate API. The viewmodel may be null in Xamarin. So we need to allow for it to be in the API?

glennawatson · 2020-07-30T03:28:11Z

Yeah, 2 is fine as long as it doesn't change existing public api (apart from nullability)

glennawatson · 2020-07-30T03:31:33Z

Thanks for putting together this PR, know there are some complications from it.

Noggog · 2020-07-30T03:41:03Z

My pleasure, I don't mind putting in the work. Part of the reason for the quick PR was to get some actual code changes to look at and discuss for clarity. I expected to have to make some adjustments on the base.

Pushed up a new commit with the ArgumentNullExceptions removed, and the appropriate members from ReactiveBinding and IPropertyBindingHook made nullable.

Seems clean enough

glennawatson · 2020-07-30T03:41:48Z

Will do some tests but this looks good. Thanks

Noggog · 2020-07-30T04:49:47Z

Are these failing tests something I need to address? Or are they part of the approval process with the different technologies?

glennawatson · 2020-07-30T04:53:46Z

If you have a windows machine run the tests and change the .received extension to .approved on the API approval tests.

glennawatson · 2020-07-30T07:51:52Z

Attempted to do this one for you but didn't have permissions. Here's a patch file for you.
api_approval.zip

Noggog · 2020-07-31T03:31:10Z

Thanks for the diff patch! I would've been quite lost looking around for those on my lonesome, haha.

gtbuchanan · 2020-08-19T16:13:42Z

Is there a particular reason this wasn't done for command bindings as well (i.e. BindCommand)? I assume it was just an oversight but just wanted to make sure before I submit another issue/PR.

cabauman · 2020-08-19T22:27:44Z

Hi @gtbuchanan . There's a related issue open #2486, and the last comment left by @glennawatson mentioned he was working on something. Maybe he can give an update.

Noggog · 2020-08-19T22:30:42Z

@gtbuchanan I didn't avoid them for any specific reason. Just an oversight as I don't typically use those calls. If they seem like a good candidate for the same refactor I'd say go for it

github-actions · 2021-11-24T00:16:11Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Noggog added 2 commits July 29, 2020 21:34

Removed unused viewmodel parameter on ExecuteHook

237817d

ViewModel objects are passed in mainly to help enforce generic typing without the user needing to manually type in all the generics themselves. Being passed to this internal functionality as object seems unnecessary

Noggog requested review from a team July 30, 2020 02:59

glennawatson closed this Jul 30, 2020

glennawatson reopened this Jul 30, 2020

glennawatson reviewed Jul 30, 2020

View reviewed changes

src/ReactiveUI/Interfaces/IPropertyBindingHook.cs Show resolved Hide resolved

glennawatson requested changes Jul 30, 2020

View reviewed changes

glennawatson changed the title ~~Bind Nullable VM Parameter~~ Fix: Make Bind() ViewModel parameters nullable. Jul 30, 2020

Noggog added 2 commits July 29, 2020 22:11

Revert "Removed unused viewmodel parameter on ExecuteHook"

41bb83f

This reverts commit 237817d.

Tied up loose ends with the commit reversal

f9170cf

Removed ArgumentNullExceptions. Offset /w some nullable params

1319854

glennawatson approved these changes Jul 30, 2020

View reviewed changes

Noggog mentioned this pull request Jul 30, 2020

Nullability Problems with Bind() calls #2467

Closed

cabauman approved these changes Jul 30, 2020

View reviewed changes

Modified API agreement text files

816e798

glennawatson merged commit 2744294 into reactiveui:main Jul 31, 2020

Noggog deleted the bind-nullable-vm-param branch August 8, 2020 05:08

github-actions bot locked as resolved and limited conversation to collaborators Nov 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Make Bind() ViewModel parameters nullable. #2468

Fix: Make Bind() ViewModel parameters nullable. #2468

Noggog commented Jul 30, 2020 •

edited

Loading

glennawatson commented Jul 30, 2020

glennawatson Jul 30, 2020

Noggog Jul 30, 2020

Noggog commented Jul 30, 2020 •

edited

Loading

glennawatson commented Jul 30, 2020

Noggog commented Jul 30, 2020 •

edited

Loading

glennawatson commented Jul 30, 2020

Noggog commented Jul 30, 2020 •

edited

Loading

glennawatson commented Jul 30, 2020

glennawatson commented Jul 30, 2020

Noggog commented Jul 30, 2020

glennawatson commented Jul 30, 2020

Noggog commented Jul 30, 2020

glennawatson commented Jul 30, 2020

glennawatson commented Jul 30, 2020

Noggog commented Jul 31, 2020

gtbuchanan commented Aug 19, 2020

cabauman commented Aug 19, 2020 •

edited

Loading

Noggog commented Aug 19, 2020

github-actions bot commented Nov 24, 2021

Fix: Make Bind() ViewModel parameters nullable. #2468

Fix: Make Bind() ViewModel parameters nullable. #2468

Conversation

Noggog commented Jul 30, 2020 • edited Loading

What kind of change does this PR introduce?

What is the new behavior?

What might this PR break?

Summary

glennawatson commented Jul 30, 2020

glennawatson Jul 30, 2020

Choose a reason for hiding this comment

Noggog Jul 30, 2020

Choose a reason for hiding this comment

Noggog commented Jul 30, 2020 • edited Loading

glennawatson commented Jul 30, 2020

Noggog commented Jul 30, 2020 • edited Loading

glennawatson commented Jul 30, 2020

Noggog commented Jul 30, 2020 • edited Loading

glennawatson commented Jul 30, 2020

glennawatson commented Jul 30, 2020

Noggog commented Jul 30, 2020

glennawatson commented Jul 30, 2020

Noggog commented Jul 30, 2020

glennawatson commented Jul 30, 2020

glennawatson commented Jul 30, 2020

Noggog commented Jul 31, 2020

gtbuchanan commented Aug 19, 2020

cabauman commented Aug 19, 2020 • edited Loading

Noggog commented Aug 19, 2020

github-actions bot commented Nov 24, 2021

Noggog commented Jul 30, 2020 •

edited

Loading

Noggog commented Jul 30, 2020 •

edited

Loading

Noggog commented Jul 30, 2020 •

edited

Loading

Noggog commented Jul 30, 2020 •

edited

Loading

cabauman commented Aug 19, 2020 •

edited

Loading